IMO this should be just panic. It can only happen if someone passes wrong value to dangerous ID creation function.

Isn't this highly misleading? If it's all-zeros it's not a nonce and thus broken. Where would one need it?

Same as here: #716 (comment)

This seems like a huge footgun.

It's also not present in the current version of the PR.

The documentation of these methods is intended for the higher-level API implementors not for for end consumers so it should rather properly describe what's going on here.

Done. Thanks.

Looks also broken.

Same as here: #716 (comment)

It looks to me that none of these Defaults should exist. People should just use arrays or MaybeUninit<T> to represent the uninitialized state.

Are you suggesting something like this ?

let key_agg_cache = MaybeUninit::<ffi::MusigKeyAggCache>::uninit();
let mut key_agg_cache = key_agg_cache.assume_init();

This will cause UB (without MaybeUninit::write).
The reason for pub fn new() is that the internal array is private (ex: pub struct MusigKeyAggCache([c_uchar; MUSIG_KEYAGG_LEN]);), which is consistent with the other structs in the code.

No, provide a function that constructs initialized types only.

Oh, now I see that I was confused because these are the FFI structs. However, I still maintain they are highly confusing.

The correct usage (inside secp256k1::musig::MusigKeyAggCache::new) is this:

let mut key_agg_cache = MaybeUninit::<ffi::MusigKeyAggCache>::uninit();
let mut agg_pk = MaybeUninit::<ffi::XOnlyPublicKey>::uninit();
unsafe {
    if ffi::secp256k1_musig_pubkey_agg(
        cx,
        agg_pk.as_mut_ptr(),
        key_agg_cache.as_mut_ptr(),
        pubkeys.as_ptr(),
        pubkey_ptrs.len(),
    ) == 0 {
        panic!(...);
    } else {
        // secp256k1_musig_pubkey_agg overwrites the cache and the key so this is sound.
        let key_agg_cache = key_agg_cache.assume_init();
        let agg_pk = agg_pk.assume_init();
        MusigKeyAggCache(key_agg_cache, pk);
    }
}

Thanks for the clarification.
Done in 2ea5674

I've also applied the same approach to the other structs.

FTR these struct declarations looked wrong but are indeed correct based on the current API.

Do you think they should be changed?

We usually name these InvalidLength.

Done. Thanks.

In f384a2d:

I (or rather, Claude) noticed that you don't have bindings to secp256k1_musig_nonce_gen_counter. Should I add one?

I'm gonna say no. Easy to add later, and this method is kinda scary.

In f384a2d:

This *mut should be *const. (This one is harmless, but we ought to fix it.)

In e90015e:

Would prefer this be called sort_pubkeys and be a method on Secp256k1. (Will do in followup.)

In dd26c30:

(Will fix in followup) there is no such error anymore. (Same below.)

In dd26c30:

I think you should panic here if the user provides all 0s. Will probably be easier to diagnose than them getting a panic later when signing. (I wouldn't bother returning a Result or something ... this function is supposed to be passed the result of a cryptographic hash or RNG or something.) (Will do in followup.)

Probably OK to do it later but this kind of thing tends to come up in cryptography more often so maybe we should have a newtype for it.

Hard to say. This function is pretty-explicitly "I got these bytes from somewhere and they're legit, promise" so it feels both paternalistic and insufficient to provide a newtype that does a single validity check on it.

Since the upstream library will do bad things if you provide all zeros, I think we should catch that, but I think that immediately panicking is probably the least bad thing to do.

I mean we could just rename the function to be less scary and make the constructor of the newtype scary with the purpose that various RNG libraries can construct it. But maybe it's not worth it here.

I think in this case we should just keep it as-is. We sorta tried this with the ThirtyTwoByteHash trait and it wound up just causing a lot of unnecessary and frustrating coupling between libraries, and also the "per-nonce" uniqueness constraint here is stronger than what a rng library can necessarily produce.

In dd26c30:

+1 to using the constant rather than just 32. Also I would like to rename this to to_byte_array for symmetry. We should do this across the codebase, but for now let's just do it with new code. (Will do in followup.)

Same rename for PublicNonce.

In dd26c30:

Needs a Panics section of the docs (will do in followup).

In dd26c30:

In the upstream docs there's an extra line clarifying that if the aggregator lies, the signature will just be invalid. We should copy that here. (Will do in followup.)

In dd26c30:

What if you pass an empty array of pubkeys? I'll play with this. Probably I'll just add an explict panic, though given that we have the same issue with partial sigs, I'm tempted to add an error variant for this, or maybe a nonemptyarray newtype. Will address in followup, at least to add a panic (I suspect the existing code will do weird potentially-UB things) but I might wind up just filing an issue.

For your curiosity, what happens is that we try to pass a bad pointer to the C code, but our alignment trickery in secp256k1-sys catches it and panics (though I doubt the panic is very reliable). So on my system it's not actually UB, but it's pretty close.

In dd26c30:

Clippy is complaining about there being new but not Default. I think it's got a point, and that we should rename this function from_thread_rng to make it more obvious that it's not a pure function. Will do in a followup.

Stale? They can just call dangerous_into_bytes and dangerous_from_bytes.

I find this wording too paranoid and a lie(?). Not implementing Copy and Clone, seems reasonable, as it will lead the user to correct usage and reading the docs, great. But once they are using something named dangerous_* it's clear that they should pay attention.

At that point seems unnecessary to tell them scary lies about MUST NOT be copied? They must not reuse the nonce, that's all, right? They can copy it a hundred times, save it, restore it, etc., as long as they not re-use it. So they have been warned and I think misinformation will just potentially confuse them.

Maybe "The nonce must not be reused, so to minimize the chance of it happening by accident this type doesn't implement Copy or Clone."

Yeah, I'm happy to tone this down (as well as the "not supported" language, which if interpreted as "you can't do it with the library" is false and if interpreted as "we won't help you" is meaningless because the software is provided as-is with no warranty).

Updated #794. If you have further comments about the text please comment there.